Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Patch structuredClone in dev to correctly clone $state proxies #14599

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Ocean-OS
Copy link
Contributor

@Ocean-OS Ocean-OS commented Dec 7, 2024

Should close #13562. Currently a draft due to lack of tests and other concerns about implementation.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Dec 7, 2024

🦋 Changeset detected

Latest commit: 31511f8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14599-svelte.vercel.app/

this is an automated message

@Ocean-OS
Copy link
Contributor Author

Ocean-OS commented Dec 7, 2024

Not sure why lint tests are failing, I ran prettier on everything I changed...

@Rich-Harris
Copy link
Member

It's erroring because export var structured_clone = structuredClone prevents Svelte from being tree-shaken (I'm not exactly sure why).

Regardless, this seems like a very bad idea — it will mean that code suddenly breaks in production. If we were to monkey-patch structuredClone in dev, the only thing it should be doing is replacing the inevitable error with a more descriptive one.

@Ocean-OS
Copy link
Contributor Author

Ocean-OS commented Dec 9, 2024

Should it throw when attempting to clone a $state proxy instead?

@Rich-Harris
Copy link
Member

I mean, it does, the error is just a bit unclear. The easy solution would be to wrap the underlying call to structuredClone in a try-catch block that inspects the error to see if it's a DataCloneError, and prints a message accordingly, but that would have false positives if you did e.g. structuredClone(new Proxy({}, {})) and the message referred to Svelte state.

The more complete solution would basically be to reimplement structuredClone in userland, including handling cycles etc.

@Ocean-OS
Copy link
Contributor Author

The more complete solution would basically be to reimplement structuredClone in userland, including handling cycles etc.

Would this also clone state proxies, or would it throw an error if attempted?

@Rich-Harris
Copy link
Member

It would throw an error. The behaviour would need to be identical to structuredClone except for the very slightly more helpful error message

@Ocean-OS
Copy link
Contributor Author

Ocean-OS commented Dec 22, 2024

@Rich-Harris I've added the more complete option (not completely finished yet) and the basic option. Most of the concerns I have are in the comments in clone.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Svelte 5: structuredClone tries to clone proxy object instead of its contents
2 participants